chore: security dependency upgrades + config adjustments by alex-gilin · Pull Request #547 · SAP/vscode-webview-rpc-lib

alex-gilin · 2026-07-02T14:07:51Z

No description provided.

bd82 · 2026-07-02T17:22:20Z


      - name: Build
        run: npm run ci
+


won't it be easier to setup a monorepo?
even npm supports workspace definition, you do not need pnpm

bd82

I started reviewing, but there are too many topics in this PR.

Eslint upgrade and config changes
TypeScript build changes
General deps upgrades
Logic changes (id++)
dangling timers handling
github actions to include pseudo sub-packages builds.
???

Maybe some (*.ts) small code changes can go together into one PR.
but most of these are completely separate topics and need separate PRs.

Ask the your AI agent to split this up for you...

bd82 · 2026-07-02T17:36:04Z

 {
  "name": "@sap-devx/webview-rpc",
-  "version": "1.1.0",
+  "version": "1.2.0",


The package version is bumped to 1.2.0 here, but the lockfiles were not regenerated: package-lock.json still has the root package at 1.1.0 and example/package-lock.json still records the local @sap-devx/webview-rpc package as 1.1.0.

Is this how the process works on this repo

bd82 · 2026-07-02T17:47:25Z

  "description": "",
  "main": "index.js",
+  "engines": {
+    "node": ">=20"


20 is already in EOL

bd82 · 2026-07-02T17:47:55Z

+      ...tseslint.configs["eslint-recommended"].overrides[0].rules,
+      // project-specific rules (preserved from original .eslintrc.json)
+      "no-unused-vars": "off",
+      "@typescript-eslint/no-unused-vars": ["error", { "argsIgnorePattern": "^_", "varsIgnorePattern": "^_" }],


AI Generated Comment To Be reviewed by Human

This relaxes the previous root lint rule. The deleted .eslintrc.json configured @typescript-eslint/no-unused-vars as plain "error", but the flat config now allows unused args and vars when they start with _. If that relaxation is intentional, it should be called out; otherwise please keep the old behavior by using @typescript-eslint/no-unused-vars: "error".

bd82 · 2026-07-02T17:48:01Z

    "compile-browser": "rimraf ./out.browser && tsc -p tsconfig.browser.json",
    "compile-ext": "rimraf ./out.ext && tsc -p ./tsconfig.ext.json",
    "compile-test": "rimraf ./out.test && tsc -p tsconfig.test.json",
-    "lint": "eslint ./**/*.ts",


AI Generated Comment To Be reviewed by Human

This narrows the root lint scope from the previous eslint ./**/*.ts to only src/**/*.ts. example-ws now has its own lint step, but example does not, so TypeScript in example/ is no longer linted by the root CI path. If the goal is an equivalent ESLint migration, please keep lint coverage for the example package or add a separate lint step for it.

bd82 · 2026-07-02T17:48:09Z

+/** @type {import("eslint").Linter.Config[]} */
+module.exports = [
+  {
+    ignores: ["node_modules/**", "out/**", "src/static/**"],


AI Generated Comment To Be reviewed by Human

The old example-ws/.eslintignore ignored only **/*.js; this flat config ignores the entire src/static/** subtree. That is more permissive because any future TypeScript added under src/static would be skipped by ESLint as well. To preserve the old behavior, please ignore only the generated/static JS files rather than the whole directory.

bd82 · 2026-07-02T17:48:15Z

    "compile": "rimraf out && tsc -p . && copyfiles -u 1 ./src/static/** ./out/",
    "test": "echo \"Error: no test specified\" && exit 1",
-    "lint": "eslint ./**/*.ts"
+    "lint": "eslint \"src/**/*.ts\""


AI Generated Comment To Be reviewed by Human

This also narrows the example-ws lint target from eslint ./**/*.ts to only src/**/*.ts. Today the TypeScript files are under src, but this is still a less comprehensive lint command than before for future .ts files added elsewhere in the example. If the migration is meant to be equivalent, please keep the previous scope or document the intentional narrowing.

bd82 · 2026-07-02T17:50:33Z

+
+Compile TypeScript and copy static assets to `out/`:
+
+```bash


is there a single script name that will run all these 4 in a sequence instead of specifying them individually?

bd82 · 2026-07-02T17:51:47Z

@@ -1,4 +1,37 @@
 "use strict";
+var __createBinding = (this && this.__createBinding) || (Object.create ? (function(o, m, k, k2) {


does this project intentionally commit generated output?

bd82 · 2026-07-02T17:57:26Z

  invoke(method: string, ...params: any[]): Promise<any> {
    const parsed = this.parseMethod(method);
-    const id = Math.random();
+    const id = ++this.nextId;


what happens when if you hit Number.MAX_SAFE_INTEGER?
Even if its unlikely it would be very hard to debug and handle, probably worth some handling and/or logging.

bd82 · 2026-07-02T18:28:29Z

suggested split by GPT 5.5.
You can probably get your AI agent to produce this split auto-magically
Hopefully they can be done truely separately

LLM Suggestion

Plan: Split PR #547 Into Focused PRs

Recommendation

Use 6 PRs, ordered so each PR has one review theme and a clear validation signal. ESLint and TypeScript should be separate because both change tooling semantics in ways that can hide subtle regressions.

PR 1: CI And Node Baseline

Scope: GitHub Actions modernization, Node 20+ baseline, release workflow npm install behavior, Dependabot config, commitlint workflow plumbing.

Include:

.github/workflows/ci.yml
.github/workflows/release.yml
.github/workflows/commitlint.yml
.github/dependabot.yml
engines fields if the repo wants to formally require Node 20+

Avoid:

Dependency mass upgrades
ESLint flat config
TypeScript config/module changes
Runtime code changes

Why first: It makes later PRs run against the intended platform and keeps workflow review separate from code behavior.

PR 2: TypeScript Upgrade And Module Resolution

Scope: TypeScript 5.x upgrade, tsconfig module/moduleResolution changes, imports needed to satisfy the new compiler settings, Jest/ts-jest adjustments needed for tests.

Include:

typescript, ts-jest, relevant @types/* updates if required
tsconfig.*.json
Import/export changes required by Node16 or bundler resolution
Generated example/out/* only if the example build output is committed and changes solely because of TS output

Avoid:

ESLint migration
Runtime behavior changes like IDs/timeouts
Security dependency updates unrelated to TS
README rewrites

Validation:

npm run compile
npm test
Example compile checks

PR 3: ESLint 9 Flat Config Migration

Scope: Convert .eslintrc.json / .eslintignore to eslint.config.cjs, update lint scripts, and preserve previous lint behavior unless explicitly documented.

Include:

Root eslint.config.cjs
example-ws/eslint.config.cjs
Deletion of old ESLint config files
ESLint-related dependencies
Lint script changes

Review focus:

No unintended relaxation of rules
No unintended narrowing of linted files
If a rule/scope is intentionally relaxed, call it out in the PR description

Specific issues to avoid from current PR:

Root _-prefixed unused vars becoming allowed without explanation
Root lint no longer covering example/**/*.ts
example-ws ignoring all src/static/** instead of only JS
example-ws lint target narrowing from all TS files to only src/**/*.ts

Validation:

npm run lint
npm run lint in example-ws
Optionally compare old/new lint target file lists

PR 4: Security And Dependency Upgrades

Scope: Upgrade dependencies for security after TS and ESLint migrations are isolated.

Include:

ws upgrade if it is treated as a dependency/security upgrade
@braintree/sanitize-url upgrade
Remaining runtime/dev dependency updates
Root and example lockfile regeneration
Minimal code changes required by dependency API changes, such as ws import/message handling adjustments

Avoid:

TypeScript config migration unless already merged
ESLint config migration unless already merged
RPC behavior changes not required by dependency APIs
Docs rewrites

Validation:

npm ci
npm audit
npm run ci
npm ci && npm audit && npm run compile in examples

PR 5: RPC Runtime Behavior Fixes

Scope: Actual library behavior changes that deserve functional review.

Include:

Replacing random request IDs with monotonic IDs
scheduleResponseTimeout() and timer unref() changes
WebSocket message parsing behavior if not already required by the ws upgrade PR
Tests for ID uniqueness, timeout behavior, and WebSocket parsing where feasible

Avoid:

Dependency mass upgrades
ESLint/TS migrations
README rewrites
Version bump

Why separate: These changes affect runtime semantics and should not be hidden inside tooling/dependency churn.

PR 6: Docs And Release Metadata

Scope: README updates, example README updates, package version bump, release notes if applicable.

Include:

README.md
example/README.md
example-ws/README.md
package.json version bump
Matching lockfile version updates

Avoid:

Code changes other than lockfile metadata needed for the version bump
Tooling migrations
Dependency upgrades

Recommendation: Keep the version bump last, after the functional and dependency PRs are merged, so the lockfiles reflect the final package metadata.

Alternative If You Want Fewer PRs

Use 5 PRs by merging PR 1 and PR 6 workflow/docs/release metadata, but I would not merge ESLint, TypeScript, dependency upgrades, or runtime behavior with each other.

Suggested Order

CI and Node baseline
TypeScript upgrade and module resolution
ESLint flat config migration
Security and dependency upgrades
RPC runtime behavior fixes
Docs and release metadata

Review Strategy

Ask the author to close PR #547 and reopen these slices, or use stacked branches if preserving current work is easier. Each PR description should explicitly list intentional behavior changes versus mechanical migrations.

alex-gilin added 4 commits July 2, 2026 13:43

chore: upgrade dependencies for security

be0c25c

chore: align TypeScript module resolution

276d022

docs: update README files

089adc8

chore: bump package version

67c8d9a

bd82 reviewed Jul 2, 2026

View reviewed changes

bd82 requested changes Jul 2, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

chore: security dependency upgrades + config adjustments#547

chore: security dependency upgrades + config adjustments#547
alex-gilin wants to merge 4 commits into
masterfrom
chore/security-dependency-upgrades

alex-gilin commented Jul 2, 2026

Uh oh!

bd82 Jul 2, 2026

Uh oh!

bd82 left a comment

Uh oh!

bd82 Jul 2, 2026

Uh oh!

bd82 Jul 2, 2026

Uh oh!

bd82 Jul 2, 2026

Uh oh!

bd82 Jul 2, 2026

Uh oh!

bd82 Jul 2, 2026

Uh oh!

bd82 Jul 2, 2026

Uh oh!

bd82 Jul 2, 2026

Uh oh!

bd82 Jul 2, 2026

Uh oh!

bd82 Jul 2, 2026

Uh oh!

bd82 commented Jul 2, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		@@ -1,4 +1,37 @@
		"use strict";
		var __createBinding = (this && this.__createBinding) \|\| (Object.create ? (function(o, m, k, k2) {

Uh oh!

Conversation

alex-gilin commented Jul 2, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bd82 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

AI Generated Comment To Be reviewed by Human

Uh oh!

Choose a reason for hiding this comment

AI Generated Comment To Be reviewed by Human

Uh oh!

Choose a reason for hiding this comment

AI Generated Comment To Be reviewed by Human

Uh oh!

Choose a reason for hiding this comment

AI Generated Comment To Be reviewed by Human

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bd82 commented Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

LLM Suggestion

Plan: Split PR #547 Into Focused PRs

Recommendation

PR 1: CI And Node Baseline

PR 2: TypeScript Upgrade And Module Resolution

PR 3: ESLint 9 Flat Config Migration

PR 4: Security And Dependency Upgrades

PR 5: RPC Runtime Behavior Fixes

PR 6: Docs And Release Metadata

Alternative If You Want Fewer PRs

Suggested Order

Review Strategy

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bd82 commented Jul 2, 2026 •

edited

Loading